Reorganize ext/uri tests - equivalence#20391
Conversation
TimWolla
left a comment
There was a problem hiding this comment.
I have looked at all WHATWG tests.
174a8bc to
7e5d097
Compare
|
Can you pleas take another look? I supposedly addressed all the review comments. |
|
@TimWolla Do you have time for review? If not, can I proceed with the merge? I think it's ready for that. |
TimWolla
left a comment
There was a problem hiding this comment.
Sorry for the delayed review, I missed the notification of the first ping and was traveling for the second one.
I'm still having some troubles with the “include fragment” vs “exclude fragment” tests, since it's hard for me to match up which “include” test belongs to which “exclude” tests (or to verify if they actually come in pairs).
I would suggest refactoring the fragment tests to always include both the include and the exclude parameter, so basically 4 “assertions”:
var_dump($uri1->equals($uri2, Uri\UriComparisonMode::ExcludeFragment));
var_dump($uri2->equals($uri1, Uri\UriComparisonMode::ExcludeFragment));
var_dump($uri1->equals($uri2, Uri\UriComparisonMode::IncludeFragment));
var_dump($uri2->equals($uri1, Uri\UriComparisonMode::IncludeFragment));
This allows one to immediately see if the fragment inclusion changes something or not.
7e5d097 to
4588860
Compare
|
I cleaned up the the fragment tests and I slightly renamed most tests. Also, I changed the target to PHP 8.5 |
TimWolla
left a comment
There was a problem hiding this comment.
If these two comments are fixed this LGTM. Thank you and sorry for taking this long to review.
* PHP-8.5: Reorganize ext/uri tests - equivalence (#20391)
No description provided.